fix(platform-wallet): age-guard the finalized-transaction handle broadcast - #4309
fix(platform-wallet): age-guard the finalized-transaction handle broadcast#4309bfoss765 wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe wallet rejects finalized transaction handles whose reservations reach 20 blocks of age. Abandonment avoids unsafe aged outpoint release. FFI mappings, Kotlin documentation, and cleanup tests cover the stale-reservation behavior. ChangesFinalized transaction reservation expiry
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CoreWallet
participant reservation_expired
participant TransactionBroadcaster
participant abandon_transaction
CoreWallet->>reservation_expired: Check finalized transaction age
reservation_expired-->>CoreWallet: Return stale or usable status
CoreWallet->>TransactionBroadcaster: Broadcast usable finalized transaction
CoreWallet->>abandon_transaction: Abandon stale finalized transaction
abandon_transaction-->>CoreWallet: Apply age-aware reservation cleanup
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit e4e6784) |
2a540f5 to
224704f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 50-55: Update the broadcast method around the reservation
validation to acquire generation_payment_guard, verify is_current_generation,
and return the appropriate stale-generation error when the wallet is no longer
current. Hold the guard through the broadcaster call so teardown cannot occur
between validation and network submission, while preserving the existing
reservation_expired check.
In `@packages/rs-platform-wallet/src/wallet/reservations.rs`:
- Around line 57-68: Correct the aged-cleanup documentation to distinguish
token-less reservations from owner-guarded reservations: in
packages/rs-platform-wallet/src/wallet/reservations.rs lines 57-68, state that
only token-less cleanup skips unguarded release while abandon_transaction can
release with an owner token; update the corresponding stale-broadcast and
release descriptions in
packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs lines 163-168,
packages/rs-platform-wallet/src/error.rs lines 103-108,
packages/rs-platform-wallet/src/test_support.rs lines 364-366,
packages/rs-platform-wallet/src/wallet/core/broadcast.rs lines 403-405,
packages/rs-platform-wallet-ffi/src/error.rs lines 276-281,
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
lines 65-71, and packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs
lines 389-395 so normal aged finalized handles are documented as owner-guarded
releases and only the token-less branch skips release.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e486da5f-6817-4ca9-a83f-f928619636b5
📒 Files selected for processing (10)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/reservations.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4309 +/- ##
=========================================
Coverage 87.80% 87.80%
=========================================
Files 2641 2641
Lines 336510 336510
=========================================
Hits 295468 295468
Misses 41042 41042
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The age check correctly prevents stale V2 transactions from reaching the broadcaster, and the new owner-guarded abandon/free behavior safely releases still-owned reservations at any age. However, the terminal FFI stale-broadcast path consumes the only transaction handle without invoking that cleanup, so an immediate rebuild can remain blocked until the reservation TTL expires. Several public comments also still describe the superseded age-based cleanup policy or omit the stale terminal outcome.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Opus: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:50-55: Use the reservation owner token when stale handles are consumed
The stale branch returns without reconciling the reservation. At the FFI boundary, `core_wallet_broadcast_signed_transaction_v2` has already removed the opaque handle, while Swift and Kotlin also clear their local handles before entering the ABI, so the caller cannot abandon it afterward. Between the 20-block guard and key-wallet's 24-block TTL, the reservation is normally still owned by this finalized build; consequently, the instructed immediate rebuild can fail because the only available input remains reserved. `abandon_transaction` now uses `release_reservation_if_owner` whenever the finalized transaction carries its owner token, safely releasing a still-owned reservation and doing nothing if a sweep or re-reservation transferred ownership. Invoke that cleanup before returning `StaleReservation`. The existing Rust test does not cover the terminal FFI behavior because it explicitly calls `abandon_transaction` after receiving the stale error.
In `packages/rs-platform-wallet/src/error.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/error.rs:101-108: StaleReservation docs describe the old abandon behavior
These comments say aged abandon/free always skips reservation release, but `CoreWallet::abandon_transaction` now skips only for token-less transactions. A normal funded finalized handle carries an owner token and attempts `release_reservation_if_owner` at every age, releasing inputs only while this build still owns them and safely doing nothing after ownership transfers. The same obsolete policy appears in `wallet/reservations.rs:57-68`, `wallet/signed_payment_registry.rs:163-168`, `test_support.rs:364-366`, `wallet/core/broadcast.rs:403-406`, `rs-platform-wallet-ffi/src/error.rs:269-281`, the FFI test comment at `rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:389-396`, and Kotlin's `ManagedCoreWallet.kt:64-71`. Update these mirrors to distinguish owner-guarded cleanup from the token-less by-outpoint fallback.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:25-34: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction_v2` can return `ErrorStaleReservationToken` code 34 after permanently consuming the opaque handle. This outcome does not touch the broadcaster, does not allocate an output txid string, and cannot be recovered by subsequently calling abandon/free with the consumed handle. The exported C-boundary documentation currently describes success, ambiguous submission, definitive rejection, and removed-wallet failure only. Document code 34 and its handle, network, txid, rebuild, and owner-guarded reservation-cleanup contract consistently with the stale-consumption fix.
…dcast Rebased down to the age-guard onto current v4.2-dev: the #4185/#4308 stack it was riding merged, and #4323/#4325 renamed the finalized- transaction surface (the v2 suffix is gone), so the guard now lands on core_wallet_broadcast_signed_transaction and the slice-based finalize_transaction signature. Mirrors the deferred registry-token age policy on the finalized-handle path: RESERVATION_MAX_AGE_BLOCKS (20; key-wallet TTL 24) and reservation_expired() live in wallet::reservations, shared by both surfaces. broadcast_finalized_transaction refuses with StaleReservation (FFI ErrorStaleReservationToken, 34) before touching the broadcaster once the reservation's stamp height has aged past the bound — and the refusal reconciles the reservation on the way out, exactly like the registry's stale-token branch: the FFI wrapper has already consumed the opaque handle, so no follow-up abandon is possible, and the owner- guarded release (safe at any age; a no-op once ownership transferred) frees the still-owned inputs for the instructed immediate rebuild. Abandon/free likewise release owner-guarded at any age, with the by-outpoint skip retained only for token-less builds. Boundary tests cover both account types on the platform and FFI layers, including the terminal FFI stale-broadcast path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
224704f to
61f871e
Compare
bfoss765
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/error.rs`:
- Around line 121-147: Fix the rustdoc link in
PlatformWalletError::StaleReservation so it does not reference the private
crate::wallet::reservations::RESERVATION_MAX_AGE_BLOCKS item. Replace that link
with a publicly reachable target, while retaining the existing public
SignedCoreTransaction::reservation_height link and the documented behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 133f9314-a352-40da-9641-f276b3b3b5e3
📒 Files selected for processing (10)
packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.ktpackages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.ktpackages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rspackages/rs-platform-wallet-ffi/src/error.rspackages/rs-platform-wallet/src/error.rspackages/rs-platform-wallet/src/test_support.rspackages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/core/transaction.rspackages/rs-platform-wallet/src/wallet/reservations.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt
- packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt
- packages/rs-platform-wallet/src/wallet/reservations.rs
- packages/rs-platform-wallet-ffi/src/error.rs
- packages/rs-platform-wallet/src/wallet/core/transaction.rs
- packages/rs-platform-wallet/src/test_support.rs
- packages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
- packages/rs-platform-wallet/src/wallet/core/broadcast.rs
…a public doc Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The stale-handle path now performs owner-guarded cleanup and has strong terminal-path coverage, but the freshness check can still race a multi-block height advance and reservation reassignment before network dispatch. The exported C documentation omits the stale terminal outcome, and Kotlin promises a typed stale error without translating the JNI exception on its public direct broadcast method.
Source: reviewers gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:55-62: Keep the reservation valid until broadcast dispatch
`last_processed_height()` releases the wallet-manager read lock before the broadcaster reaches network dispatch. The FFI lifecycle guard excludes wallet teardown, but it does not exclude sync updates or concurrent finalization because payment guards are shared. A call can therefore sample the reservation at age 19, yield in the broadcaster while catch-up advances the wallet to age 24, and then race a new finalization that triggers key-wallet's TTL sweep and reserves the same input under a new token. The old signed transaction can subsequently be submitted against that reassigned UTXO. The four-block margin reduces ordinary likelihood but does not establish an ordering invariant because catch-up can advance multiple blocks. Atomically validate ownership and pin or mark the reservation as in-broadcast under the same synchronization used by height advancement and coin selection, keeping that state until dispatch has definitively begun. The registry-token broadcast uses the same check-then-dispatch pattern and should use the same primitive.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but its exported contract still lists only ordinary broadcast and removed-wallet outcomes. On the stale branch, Rust has already consumed the opaque handle, leaves `out_txid` null, never invokes the broadcaster, and performs owner-guarded reservation cleanup so the caller can rebuild immediately. Native callers need these terminal ownership and recovery semantics explicitly documented; retrying, abandoning, or freeing the consumed handle is not valid.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt`:
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/wallet/ManagedCoreWallet.kt:45-49: Translate the stale JNI error promised by the Kotlin API
The public method documents that stale broadcast throws `DashSdkError.PlatformWallet.StaleReservationToken`, but it invokes the external JNI method directly. JNI turns native code 34 into the internal `DashSDKException`; without `mapNativeErrors`, direct callers of `coreWallet().broadcastTransaction(...)` receive that internal exception rather than the documented public type. `sendToAddresses` happens to wrap this call from outside, but `coreWallet()` and `broadcastTransaction` are themselves public, so that outer wrapper is not an API-wide invariant.
…roadcastTransaction The method documents DashSdkError.PlatformWallet.StaleReservationToken but called the JNI native directly, so direct callers received the internal DashSDKException instead of the documented public type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pre-checked age is not an ordering invariant: between the check and the broadcaster await, sync catch-up can advance last_processed_height past the bound and a concurrent finalization can trigger key-wallet's TTL sweep, re-reserving the same inputs under a new token — the old signed transaction then hits the wire against reassigned UTXOs. New shared primitive dispatch_unexpired performs the age check and reaches the broadcaster under ONE wallet-manager READ guard. Both writers this orders against — the ReservationSet TTL sweep (inside coin selection) and height advancement — mutate under the manager WRITE lock, so 'the reservation is unexpired' and 'dispatch has begun' become a single atomic observation. Ownership needs no separate probe: the key-wallet TTL exceeds RESERVATION_MAX_AGE_BLOCKS on the same clock, so an unexpired reservation cannot already have been swept. Both check-then-dispatch sites now route through it: the finalized- handle broadcast and the registry-token broadcast (whose composite gains the reservation height and returns the stale verdict for the registry's existing owner-guarded reconciliation). Reconciliation runs OUTSIDE the guard — those paths retake manager locks. Deliberate cost: writers queue behind the network await, bounded by the broadcaster's own timeout — the price of the invariant without a key-wallet-side in-broadcast pin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- Around line 47-60: The dispatch_unexpired method currently holds the
wallet_manager read guard across the asynchronous broadcast, risking blocked
writes and re-entrant deadlocks. Add the required key-wallet in-broadcast pin
while the manager guard is held, then release the guard before awaiting
broadcaster.broadcast; also configure an explicit timeout for the
DapiBroadcaster request instead of relying on RequestSettings::default().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e758b3e3-a12c-46d9-93b3-027dcab04e8a
📒 Files selected for processing (2)
packages/rs-platform-wallet/src/wallet/core/broadcast.rspackages/rs-platform-wallet/src/wallet/signed_payment_registry.rs
| pub(crate) async fn dispatch_unexpired( | ||
| &self, | ||
| reservation_height: u32, | ||
| transaction: &Transaction, | ||
| ) -> GuardedDispatch { | ||
| let wm = self.wallet_manager.read().await; | ||
| let height = wm | ||
| .get_wallet_and_info(&self.wallet_id) | ||
| .map(|(_, info)| info.core_wallet.last_processed_height()); | ||
| if reservation_expired(reservation_height, height) { | ||
| return GuardedDispatch::Stale; | ||
| } | ||
| GuardedDispatch::Sent(self.broadcaster.broadcast(transaction).await) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# All TransactionBroadcaster implementations.
ast-grep run --lang rust \
--pattern $'impl $$$ TransactionBroadcaster for $T { $$$ }' \
packages/rs-platform-wallet packages/rs-platform-wallet-ffi || true
# Does any broadcaster body reach the wallet manager?
rg -nP -C 6 '\bwallet_manager\b' --type=rust \
-g '**/broadcaster*' -g '**/*spv*' packages/rs-platform-wallet || true
# Is a send timeout enforced on the broadcast path?
rg -nP -C 4 '(timeout|Duration::from_(secs|millis))' --type=rust \
-g '**/broadcaster*' -g '**/*spv*' packages/rs-platform-wallet || trueRepository: dashpay/platform
Length of output: 21429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dispatch_unexpired and related call sites ---'
ast-grep outline packages/rs-platform-wallet/src/wallet/core/broadcast.rs
rg -n -C 8 'dispatch_unexpired|wallet_manager' packages/rs-platform-wallet/src/wallet/core packages/rs-platform-wallet/src/wallet/asset_lock packages/rs-platform-wallet/src/wallet -g '*.rs' | head -n 260
printf '%s\n' '--- broadcaster implementation and timeout configuration ---'
ast-grep outline packages/rs-platform-wallet/src/broadcaster.rs
sed -n '1,230p' packages/rs-platform-wallet/src/broadcaster.rs
printf '%s\n' '--- lock type and manager ownership ---'
rg -n -C 5 'wallet_manager\s*:' packages/rs-platform-wallet/src -g '*.rs'
rg -n -C 5 'struct .*Wallet|type .*WalletManager|WalletManager' packages/rs-platform-wallet/src/wallet packages/rs-platform-wallet/src -g '*.rs' | head -n 220
printf '%s\n' '--- dependency and SDK request timeout settings ---'
rg -n -C 5 'RequestSettings|timeout|connect_timeout|request_timeout|DapiBroadcaster' packages/rs-platform-wallet packages -g 'Cargo.toml' -g '*.rs' | head -n 320Repository: dashpay/platform
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- CoreWallet manager field and exact guard implementation ---'
rg -n -C 12 'wallet_manager:|struct CoreWallet|impl<.*CoreWallet|dispatch_unexpired' \
packages/rs-platform-wallet/src/wallet/core packages/rs-platform-wallet/src -g '*.rs' | head -n 260
printf '%s\n' '--- Manager-accessing broadcaster test and its call sites ---'
sed -n '1360,1420p' packages/rs-platform-wallet/src/wallet/asset_lock/build.rs
rg -n -C 10 'RejectAfterConcurrentResumeBroadcaster|dispatch_unexpired|broadcast_finalized_transaction' \
packages/rs-platform-wallet/src/wallet/asset_lock/build.rs packages/rs-platform-wallet/src/wallet/core/broadcast.rs -g '*.rs'
printf '%s\n' '--- All broadcaster bodies that reference manager state ---'
python3 - <<'PY'
from pathlib import Path
import re
for path in Path("packages/rs-platform-wallet").rglob("*.rs"):
text = path.read_text()
for match in re.finditer(r"impl\s+TransactionBroadcaster\s+for\s+([^{]+)\{", text):
start = match.start()
brace = text.find("{", start)
depth = 0
end = None
for i in range(brace, len(text)):
if text[i] == "{":
depth += 1
elif text[i] == "}":
depth -= 1
if depth == 0:
end = i + 1
break
body = text[start:end]
print(f"{path}:{text.count(chr(10), 0, start)+1}: {match.group(1).strip()}")
for line_no, line in enumerate(body.splitlines(), text.count(chr(10), 0, start)+1):
if "wallet_manager" in line or ".read()" in line or ".write()" in line:
print(f" {line_no}: {line.strip()}")
PY
printf '%s\n' '--- DAPI request settings definitions available in the repository ---'
rg -n -C 8 'struct RequestSettings|impl Default for RequestSettings|RequestSettings' \
. -g '*.rs' -g 'Cargo.toml' -g 'Cargo.lock' | head -n 260
printf '%s\n' '--- dependency versions ---'
rg -n -C 3 'dash-sdk|dash-spv' Cargo.toml Cargo.lock packages -g 'Cargo.toml' -g 'Cargo.lock' | head -n 180Repository: dashpay/platform
Length of output: 50372
🌐 Web query:
dash-sdk Rust RequestSettings default timeout DapiRequestExecutor execute RequestSettings
💡 Result:
In the Dash SDK for Rust, RequestSettings defines the timeout and retry behavior for DAPI requests [1][2]. Default Settings The SDK defines a default configuration within its source code [3]. While RequestSettings provides a default method that creates an empty instance (where fields are None), the actual default values used by the SDK when no specific settings are provided are defined as follows [1][4][3]: const DEFAULT_REQUEST_SETTINGS: RequestSettings = RequestSettings { retries: Some(3), timeout: None, ban_failed_address: None, connect_timeout: None, }; As shown above, the default timeout and connect_timeout are None, meaning no specific timeout is applied by default [1][3]. Settings Hierarchy The SDK uses a four-level hierarchy to determine the effective settings for a request, where each subsequent level overrides the previous ones [1][5][4]: 1. Library defaults (as defined in DEFAULT_REQUEST_SETTINGS) [4][3]. 2. DapiClient settings (configured via SdkBuilder) [4][3]. 3. DapiRequest-specific settings (defined by the specific request type) [1][6][7]. 4. Settings provided during an exact request execution call (passed to execute) [1][4]. Execution via DapiRequestExecutor The DapiRequestExecutor trait, implemented by the Sdk struct, includes the execute method [8][3]. When you call execute, you pass the request and an instance of RequestSettings [8][3]. The executor then calculates the effective settings by merging the provided settings with the existing client-level configuration (e.g., using an override_by mechanism) [4][3]. To use custom timeouts, you should provide a RequestSettings instance with the desired Duration values for connect_timeout or timeout [1].
Citations:
- 1: https://dashpay.github.io/docs-platform/dash_sdk/sdk/struct.RequestSettings.html
- 2: https://dashpay.github.io/platform/api/rust/rs_dapi_client/index.html
- 3: https://dashpay.github.io/docs-platform/src/dash_sdk/sdk.rs.html
- 4: https://dashpay.github.io/platform/sdk/builder-pattern.html
- 5: https://dashpay.github.io/docs-platform/dash_sdk/struct.RequestSettings.html
- 6: https://dashpay.github.io/docs-platform/dash_sdk/platform/types/identity/enum.IdentityRequest.html
- 7: https://dashpay.github.io/docs-platform/dash_sdk/platform/proto/struct.GetDocumentsRequest.html
- 8: https://dashpay.github.io/docs-platform/dash_sdk/sdk/struct.Sdk.html
Release wallet_manager before the network await.
DapiBroadcaster passes RequestSettings::default(), which has no timeout unless the SDK client overrides it. A stalled request can hold the shared manager read guard indefinitely. SpvBroadcaster can hold it for up to SPV_ACCEPTANCE_TIMEOUT (30 seconds), blocking manager writes and unrelated reads. The TransactionBroadcaster trait also permits re-entering the manager; RejectAfterConcurrentResumeBroadcaster acquires its write lock and would deadlock in this path. Add an explicit DAPI timeout and use a key-wallet-side in-broadcast pin before releasing the manager guard.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/rs-platform-wallet/src/wallet/core/broadcast.rs` around lines 47 -
60, The dispatch_unexpired method currently holds the wallet_manager read guard
across the asynchronous broadcast, risking blocked writes and re-entrant
deadlocks. Add the required key-wallet in-broadcast pin while the manager guard
is held, then release the guard before awaiting broadcaster.broadcast; also
configure an explicit timeout for the DapiBroadcaster request instead of relying
on RequestSettings::default().
There was a problem hiding this comment.
The lock-across-await issue is valid, and the production SPV path makes it more severe than writer delay alone. Dash-SPV dispatches the local transaction, then its serialized mempool task blocks on wallet.write() before it can process the peer echo, InstantSend event, or confirmation awaited by this call, so fresh broadcasts can reach peers and still time out as MaybeSent. One correction: DAPI's empty RequestSettings is ultimately finalized with the dapi-client's 10-second per-request default, so it is not inherently unbounded, although retries can still hold this global guard for an extended period.
There was a problem hiding this comment.
@thepastaclaw, thank you for the correction and the SPV execution detail.
The lock-across-await finding remains valid. The SPV path can cause a broadcast to reach peers and then return MaybeSent because the held read guard blocks the mempool task's required wallet.write().
I retract the claim that the DAPI request is inherently unbounded. The dapi-client applies a 10-second per-request default. Retries can still keep the wallet-manager read guard for an extended period.
The required design remains: establish a key-wallet in-broadcast pin while the manager guard is held, release the guard, then await the broadcaster.
🐇
✏️ Learnings added
Learnt from: thepastaclaw
Repo: dashpay/platform PR: 4309
File: packages/rs-platform-wallet/src/wallet/core/broadcast.rs:47-60
Timestamp: 2026-08-10T20:43:27.089Z
Learning: In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`, `CoreWallet::dispatch_unexpired` must not hold the wallet-manager read guard across `TransactionBroadcaster::broadcast`. In the production Dash-SPV path, the broadcaster dispatches the transaction and then waits for processing that requires the serialized mempool task to acquire `wallet.write()`. A held read guard can block that write, so a fresh transaction can reach peers but return `MaybeSent`.
Learnt from: thepastaclaw
Repo: dashpay/platform PR: 4309
File: packages/rs-platform-wallet/src/wallet/core/broadcast.rs:47-60
Timestamp: 2026-08-10T20:43:27.089Z
Learning: For the DAPI broadcaster in the platform wallet, an empty `RequestSettings` is finalized by dapi-client with a 10-second per-request default. It is not inherently unbounded, but retry behavior can extend the total time that a broadcast call occupies a wallet-manager read guard.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The previous freshness race is closed, but the replacement holds the shared wallet-manager read lock while the production SPV broadcaster waits for acceptance. Dash-SPV must acquire the same manager's write lock before its serialized mempool task can process the acceptance signals, so fresh transactions can reach peers yet consistently time out as MaybeSent; the exported FFI documentation also still omits the terminal stale outcome.
Source: reviewers gpt-5.6-sol (general, security-auditor, rust-quality, ffi-engineer); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/core/broadcast.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/wallet/core/broadcast.rs:52-59: Release the manager lock before awaiting SPV acceptance
`dispatch_unexpired` retains the shared wallet-manager read guard throughout `TransactionBroadcaster::broadcast`. The production `SpvBroadcaster` does not return when initial dispatch begins: it calls dash-spv's `broadcast_transaction_and_wait` and waits up to 30 seconds for a peer echo, InstantSend lock, or confirmation. `SpvRuntime` was constructed with this same wallet manager. Dash-SPV's local transaction handler first sends the transaction to selected peers and then calls `wallet.write().await` before `process_mempool_transaction`; that write cannot proceed while this read guard is held. Because the mempool manager handles its local transaction, peer messages, and sync events serially, it also cannot process the later echo, InstantSend, or confirmation that would resolve the waiting broadcast. A fresh transaction can therefore reach peers but time out as `MaybeSent`, retaining its reservation and reporting an ambiguous failure instead of success. The same guard also delays all manager writers during DAPI or SPV network I/O. Preserve freshness and ownership with a reservation-level in-broadcast pin installed under the manager lock, or split initial dispatch from acceptance waiting, then release the manager guard as soon as network dispatch has definitively begun.
In `packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs`:
- [SUGGESTION] packages/rs-platform-wallet-ffi/src/core_wallet/broadcast.rs:24-33: Broadcast FFI docs omit the stale terminal outcome
`core_wallet_broadcast_signed_transaction` can return `ErrorStaleReservationToken` (34), but its exported contract documents only ordinary broadcast and removed-wallet outcomes. The function removes the opaque transaction handle before validation. On the stale branch, the broadcaster is never invoked, `out_txid` remains null, and owner-guarded cleanup releases any reservation still owned by this transaction so the caller can rebuild immediately. Raw native callers need to know that this outcome is terminal: retrying, abandoning, or freeing the consumed handle is invalid.
| let wm = self.wallet_manager.read().await; | ||
| let height = wm | ||
| .get_wallet_and_info(&self.wallet_id) | ||
| .map(|(_, info)| info.core_wallet.last_processed_height()); | ||
| if reservation_expired(reservation_height, height) { | ||
| return GuardedDispatch::Stale; | ||
| } | ||
| GuardedDispatch::Sent(self.broadcaster.broadcast(transaction).await) |
There was a problem hiding this comment.
🔴 Blocking: Release the manager lock before awaiting SPV acceptance
dispatch_unexpired retains the shared wallet-manager read guard throughout TransactionBroadcaster::broadcast. The production SpvBroadcaster does not return when initial dispatch begins: it calls dash-spv's broadcast_transaction_and_wait and waits up to 30 seconds for a peer echo, InstantSend lock, or confirmation. SpvRuntime was constructed with this same wallet manager. Dash-SPV's local transaction handler first sends the transaction to selected peers and then calls wallet.write().await before process_mempool_transaction; that write cannot proceed while this read guard is held. Because the mempool manager handles its local transaction, peer messages, and sync events serially, it also cannot process the later echo, InstantSend, or confirmation that would resolve the waiting broadcast. A fresh transaction can therefore reach peers but time out as MaybeSent, retaining its reservation and reporting an ambiguous failure instead of success. The same guard also delays all manager writers during DAPI or SPV network I/O. Preserve freshness and ownership with a reservation-level in-broadcast pin installed under the manager lock, or split initial dispatch from acceptance waiting, then release the manager guard as soon as network dispatch has definitively begun.
source: ['codex', 'coderabbit']
2 participants
Continues #4196 — moved from a fork branch to an in-repo branch so maintainers can push changes directly, per review request. Full review history on #4196.
Rebased down to just the age-guard onto current
v4.2-dev(2026-08-10): the #4185/#4308 stack this PR was riding has merged, so every stacked commit was dropped and the single age-guard commit was adapted to the renamed finalized-transaction surface (#4323/#4325 removed the_v2/V2 suffixes) and the slice-basedfinalize_transactionsignature.Follow-up to #4185 (requested by @shumkov): the finalized-transaction
handle surface (
core_wallet_tx_builder_finalize→broadcast_finalized_transaction) retained a stale-release hazard — a pinnedhandle had no age guard, so a long-held handle could broadcast against
funding inputs that key-wallet's
ReservationSetTTL sweep may already havereleased and re-selected for an unrelated build. This goes live the moment iOS
starts issuing deferred sends.
This mirrors the deferred registry-token age policy on the finalized-handle path:
RESERVATION_MAX_AGE_BLOCKS(20; key-wallet TTL 24) andreservation_expired()are hoisted towallet::reservations, so theregistry and the finalized-handle path measure a reservation's age against
the same number.
broadcast_finalized_transactionrefuses — beforetouching the broadcaster — once
current_height − reservation_height >=theshared bound, using the reservation's own stamp height already carried on
SignedCoreTransaction::reservation_height. The check runs after theexisting generation-identity check, matching the registry order. The refusal
reconciles the reservation on the way out, exactly like the registry's
stale-token branch: the FFI wrapper has already consumed the opaque handle,
so no follow-up abandon is possible, and the owner-guarded release
(
release_reservation_if_owner, safe at any age — a no-op once ownershiptransferred) frees the still-owned inputs for the instructed immediate
rebuild.
PlatformWalletError::StaleReservationreuses the existing FFI
ErrorStaleReservationToken(34); no new code isallocated. Reuse is documented on both sides.
(never reached on the funded finalize path) honours the bound and skips its
unguarded by-outpoint release, leaving the aged reservation for key-wallet's
TTL to reclaim.
Tests: fresh handle broadcasts; aged handle refuses with
StaleReservationand the refusal itself releases for an immediate rebuild (a late abandon of the
consumed handle is an owner-guarded no-op that cannot free the rebuild's
reservation); exact threshold boundary (BIP44/BIP32); FFI mapping to the shared
code; terminal FFI stale-broadcast, aged free, and aged failure-path abandon.
Summary by CodeRabbit
Bug Fixes
Documentation
Tests